feat: Add support for tracking arbitrary events #136
Conversation
| private flagEvaluationDetailsBuilder(flagKey: string): FlagEvaluationDetailsBuilder { | ||
| // noinspection JSUnusedGlobalSymbols | ||
| track(event: unknown, params: Record<string, unknown>) { | ||
| this.eventQueue.push({ event, params }); |
There was a problem hiding this comment.
this implementation to be fleshed out in future PRs
| private isObfuscated = false, | ||
| ) {} | ||
| constructor({ | ||
| eventQueue = new ArrayBackedNamedEventQueue('events'), |
There was a problem hiding this comment.
thanks for the named parameters.
🤔 I do NOT consider this a breaking change as users are not using this constructor directly; they are going through init. Am I misunderstanding?
There was a problem hiding this comment.
I think that's correct sir!
There was a problem hiding this comment.
According to strict semver, this does require a major version bump, however, as you pointed out, since it's only used internally, it doesn't affect end users
There was a problem hiding this comment.
Nice, was worried about this as well
| let result: ContextAttributes; | ||
| if (this.isInstanceOfContextualAttributes(subjectAttributes)) { | ||
| result = subjectAttributes as ContextAttributes; | ||
| return subjectAttributes as ContextAttributes; |
| } else { | ||
| // If no logger defined, queue up the events (up to a max) to flush if a logger is later defined | ||
| this.queuedBanditEvents.push(banditEvent); | ||
| this.banditEventsQueue.push(banditEvent); |
There was a problem hiding this comment.
the limit is now encapsulated inside the logic of BoundedEventQueue#push method!
| } | ||
|
|
||
| private logAssignment(result: FlagEvaluation) { | ||
| private maybeLogAssignment(result: FlagEvaluation) { |
| export class BoundedEventQueue<T> { | ||
| constructor( | ||
| private readonly queue: NamedEventQueue<T>, | ||
| private readonly maxSize = MAX_EVENT_QUEUE_SIZE, |
There was a problem hiding this comment.
I see the bandit event queue above and makes sense now 👍
|
|
||
| try { | ||
| if (this.assignmentLogger) { | ||
| this.assignmentLogger.logAssignment(event); |
There was a problem hiding this comment.
Are you considering any more major changes to the SDK behavior, such as all events getting put into a queue and processed by another worker?
There was a problem hiding this comment.
yeah, we'll need some mechanism for batching and uploading events. I'm thinking a simple setInterval logic that uploads every N seconds, no need to overcomplicate. Thoughts?
|
|
||
| /** A named event queue backed by an array. */ | ||
| export default class ArrayBackedNamedEventQueue<T> implements NamedEventQueue<T> { | ||
| private readonly events: T[] = []; |
There was a problem hiding this comment.
Having an unbounded memory-based array sounds risky. Maybe this should be limited to a certain size if/until we support spillover to disk?
There was a problem hiding this comment.
I see there is a separate class for limiting the size. Could we combine them to simplify and avoid the risk of someone using this class directly?
There was a problem hiding this comment.
this class was not meant to be used directly, only internally from BoundedEventQueue via composition. I'll add a comment to make that clear, good callout
Towards: FF-3567
Motivation and Context
Laying the groundwork for event ingestion and logging
Description
NamedEventQueueabstraction, to be shared between assignment, bandit and arbitrary application events.EppoClientconstructor to use props object pattern with optional argsSdkKeyDecoderto be used in follow up PRsThe js-server-sdk and js-client-sdk will have separate implementations for
NamedEventQueue, one based on file storage and the other based on localstorage. I already have both implemented, will submit those as follow up PRs.Event batching, uploading, etc. will be handled in follow up PRs as well.
How has this been tested?
Existing tests